Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cover Art Fetcher: allow applying just the found cover #11938

Merged
merged 3 commits into from
Sep 28, 2023

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Sep 8, 2023

... + some layouts tweaks

before:
image

this PR:
image

Closes #11086

@Swiftb0y
Copy link
Member

Swiftb0y commented Sep 8, 2023

The status message seems clunky, couldn't that be conveyed better by greying out the apply button when its not possible to apply and then updating the "current cover art" preview if it was successfully applied?

@ronso0
Copy link
Member Author

ronso0 commented Sep 9, 2023

greying out the apply button when its not possible to apply and then updating the "current cover art" preview if it was successfully applied?

Thank you, that slipped through. IIRC I already fixed that in #11112, I'll look into either backporting the fix or do it manually in DlgTagFetcher.

@ronso0
Copy link
Member Author

ronso0 commented Sep 10, 2023

Actually, the 'Current Cover' should be updated after it was written to the track file succesfully (need to click Okay to overwrite the track file).
The status message is displayed already when the file overwrite dialog pops up.

@ronso0
Copy link
Member Author

ronso0 commented Sep 10, 2023

Furthermore, there are some inconsistencies in the file update process: the tags are written directly whereas the cover art update is initiated but results (dialog/success/failure) are processed via signals/slots, so there's no guarantee the messages are displayed in the correct order.

  • "Selected metadata applied" message may be displayed after the cover was written
  • "Metadata & Cover Art applied" is displayed by slotWorkerCoverArtUpdated even though that slot is responsible only for the cover art (until now, it was assumed that both tags and cover are updated simultaneously).

I'll check whether that can be fixed while keeping the tr strings in place.

@ronso0 ronso0 force-pushed the cover-fetcher-apply-only-cover branch from fa5ea5d to d619183 Compare September 27, 2023 21:43
src/library/dlgtagfetcher.cpp Outdated Show resolved Hide resolved
src/library/dlgtagfetcher.cpp Outdated Show resolved Hide resolved
src/library/dlgtagfetcher.cpp Show resolved Hide resolved
@ronso0 ronso0 force-pushed the cover-fetcher-apply-only-cover branch from d619183 to efe82fa Compare September 28, 2023 07:32
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you.

@Swiftb0y Swiftb0y merged commit 6a72c51 into mixxxdj:2.4 Sep 28, 2023
13 checks passed
ronso0 added a commit to ronso0/mixxx that referenced this pull request Sep 28, 2023
this removes the message added in mixxxdj#11938 and shifts the cover/cover+tags message to the correct spots
@ronso0
Copy link
Member Author

ronso0 commented Sep 28, 2023

Thanks for your review!

Furthermore, there are some inconsistencies in the file update process: the tags are written directly whereas the cover art update is initiated but results (dialog/success/failure) are processed via signals/slots, so there's no guarantee the messages are displayed in the correct order.

* "Selected metadata applied" message may be displayed after the cover was written

* "Metadata & Cover Art applied" is displayed by `slotWorkerCoverArtUpdated` even though that slot is responsible only for the cover art (until now, it was assumed that both tags and cover are updated simultaneously).

Follow-up is #12041

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants